Remove need of project.afterEvaluate() and add generateJavadoc macro task #57

bastienpaulfr · 2018-05-09T09:32:43Z

Hi,

I have made some changes to javadoc plugin. Could please review these changes and make some comments about it ?

build.gradle

I am now using plugins dsl to ease read.

I also changed the dependency to android gradle plugin with compileOnly configuration. This prevents this lib to be added to dependencies in pom.xml

Generation.groovy

I choose to not add the plugin to child projects in case it is added to a root project of multi module build. Please advise me if it is a bad idea.

I also removed the afterEvaluate closure in favor of variant.all ones.

I am also creating root task named generateJavadoc. Running this task will run javadoc task for all variants. The same for the Jar.

Last but not least : this plugin can be added before android plugin is applied.

Thanks

codecov · 2018-05-09T09:36:07Z

Codecov Report

Merging #57 into master will increase coverage by 13%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##             master      #57    +/-   ##
==========================================
+ Coverage      9.09%   22.09%   +13%     
- Complexity        3        8     +5     
==========================================
  Files             2        2            
  Lines            66       86    +20     
  Branches          7        9     +2     
==========================================
+ Hits              6       19    +13     
- Misses           60       63     +3     
- Partials          0        4     +4

Impacted Files	Coverage Δ	Complexity Δ
.../javadoc/extensions/AndroidJavadocExtension.groovy	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...y/com/vanniktech/android/javadoc/Generation.groovy	`23.45% <40%> (+13.62%)`	`8 <3> (+5)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7693ff...cd296a6. Read the comment docs.

vanniktech · 2018-05-11T08:26:21Z

README.md

@@ -2,7 +2,7 @@

 Gradle plugin that generates Java Documentation from an Android Gradle project.

-Works with the latest Gradle Android Tools version 3.0.1.
+Works with the latest Gradle Android Tools version 3.1.2.


Can you revert? I'll update the README once I've published a new version

vanniktech · 2018-05-11T08:26:26Z

README.md

@@ -31,20 +31,34 @@ buildscript {
    maven { url "https://oss.sonatype.org/content/repositories/snapshots" }
  }
  dependencies {
-    classpath "com.vanniktech:gradle-android-javadoc-plugin:0.3.0-SNAPSHOT"
+    classpath "com.vanniktech:gradle-android-javadoc-plugin:0.3.1-SNAPSHOT"


no need to change

vanniktech · 2018-05-11T08:26:42Z

README.md

  }
 }

 apply plugin: "com.vanniktech.android.javadoc"
 ```

+or with plugins dsl from version `0.3.1-SNAPSHOT`


No need for this either as it's in the gradle link

vanniktech · 2018-05-11T08:27:14Z

gradle.properties

@@ -1,5 +1,5 @@
 GROUP=com.vanniktech
-VERSION_NAME=0.3.0-SNAPSHOT
+VERSION_NAME=0.3.1-SNAPSHOT


no need to change

vanniktech · 2018-05-11T08:27:47Z

src/main/groovy/com/vanniktech/android/javadoc/Generation.groovy

-                    applyPluginToProject(p);
-                } else {
-                    logger.info "${p.name} is not an android project - plugin is not applied"
+        // Do not apply in root project anymore. User will have to apply plugin manually in all child project.


I'm actually a fan of having it on the root project and then apply everywhere else.

Why do you want to change it?

Ok, you convince me. I've put again the project.allProject{}

vanniktech · 2018-05-11T08:28:00Z

src/main/groovy/com/vanniktech/android/javadoc/Generation.groovy

            addJavaTaskToProjectWith(project, (DomainObjectCollection<BaseVariant>) project.android.libraryVariants)
        } else {
            // Do not throw exception anymore to support big projects with a lot of modules that not all of them are android.
            logger.info "${project.name} has no application or library variant - plugin is not applied"
        }
    }

+    private void createRootTask(Project project) {
+        if (project.tasks.findByPath(JAVADOC_TASK)) {
+            logger.debug "task $JAVADOC_TASK already exists"


how can this happen?

It can happen if user create a task with the same name or if he applies a plugin that create a task with the same name.

It is still very unlikely...

vanniktech · 2018-05-11T08:28:06Z

src/main/groovy/com/vanniktech/android/javadoc/Generation.groovy

+            }
+        }
+        if (project.tasks.findByPath(JAVADOC_JAR_TASK)) {
+            logger.debug "task $JAVADOC_JAR_TASK already exists"


vanniktech · 2018-05-11T08:28:35Z

src/main/groovy/com/vanniktech/android/javadoc/Generation.groovy

            group = 'Documentation'

            destinationDir = getJavadocFolder(project, variant)
            source = variant.javaCompiler.source

-            ext.androidJar = "${project.android.sdkDirectory}/platforms/${project.android.compileSdkVersion}/android.jar"


what happened to the android.jar?

This has been replaced by project.android.getBootClasspath()

It is used in the doFirst{} closure just bellow.

vanniktech · 2018-05-16T07:21:17Z

build.gradle


+  compileOnly 'com.android.tools.build:gradle:3.1.2'
+
+  testCompile 'com.android.tools.build:gradle:3.1.2'


Do we need this when using compileOnly? I'm not sure

Yes we need this, Unit tests are pretty much clear ^^
Unit tests do need the android dependency at runtime also

vanniktech · 2018-05-16T07:23:15Z

src/main/groovy/com/vanniktech/android/javadoc/Generation.groovy

-                    applyPluginToProject(p);
-                } else {
-                    logger.info "${p.name} is not an android project - plugin is not applied"
+        if (isRoot(project)) {


Can you change it to this? I'm also using the same code in other gradle projects and would like to be consistent:

def hasSubProjects = project.subprojects.size() > 0 if (hasSubProjects) { project.subprojects { subProject -> applyPlugin(subProject) } } else { applyPlugin(project) }

vanniktech · 2018-05-16T07:23:39Z

src/main/groovy/com/vanniktech/android/javadoc/Generation.groovy

+    }
+
+    private void applyPlugin(Project project) {
+        if (project.hasProperty('android')) {


I don't think we need this at all when using the whenPluginAdded method. Less code and clearer

Yes we need this, because whenPluginAdded is not called if plugin has been applied before this one

vanniktech · 2018-05-16T07:24:10Z

src/main/groovy/com/vanniktech/android/javadoc/Generation.groovy

+        } else {
+            // Waiting for android plugin to be applied
+            project.plugins.whenPluginAdded { Plugin plugin ->
+                if (project.plugins.hasPlugin("com.android.library")


Should we also support the other android plugins? If so you can use:

private static boolean isAndroidProject(final Project project) { final boolean isAndroidLibrary = project.plugins.hasPlugin('com.android.library') final boolean isAndroidApp = project.plugins.hasPlugin('com.android.application') final boolean isAndroidTest = project.plugins.hasPlugin('com.android.test') final boolean isAndroidFeature = project.plugins.hasPlugin('com.android.feature') final boolean isAndroidInstantApp = project.plugins.hasPlugin('com.android.instantapp') return isAndroidLibrary || isAndroidApp || isAndroidTest || isAndroidFeature || isAndroidInstantApp }

I have replaced this by plugin instanceOf com.android.build.gradle.BasePlugin
It cover all other android plugins.

Also, there was an issue because plugin was applied each time a new plugin was applied after android ones. Now Javadoc plugin is applied only once.

vanniktech

Looks much better. Some final notes and then we can merge

…task

vanniktech

Thanks!

vanniktech reviewed May 11, 2018

View reviewed changes

bastienpaulfr force-pushed the dev branch 2 times, most recently from 829d83a to ae7bda3 Compare May 14, 2018 06:20

vanniktech reviewed May 16, 2018

View reviewed changes

Remove need of project.afterEvaluate() and add generateJavadoc macro …

cd296a6

…task

bastienpaulfr force-pushed the dev branch from ae7bda3 to cd296a6 Compare May 16, 2018 08:57

vanniktech approved these changes May 16, 2018

View reviewed changes

vanniktech merged commit 661f497 into vanniktech:master May 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove need of project.afterEvaluate() and add generateJavadoc macro task #57

Remove need of project.afterEvaluate() and add generateJavadoc macro task #57

bastienpaulfr commented May 9, 2018 •

edited

Loading

codecov bot commented May 9, 2018 •

edited

Loading

vanniktech May 11, 2018

bastienpaulfr May 14, 2018

vanniktech May 11, 2018

bastienpaulfr May 14, 2018

vanniktech May 11, 2018

vanniktech May 11, 2018

vanniktech May 11, 2018

bastienpaulfr May 14, 2018

vanniktech May 11, 2018

bastienpaulfr May 14, 2018

vanniktech May 11, 2018

vanniktech May 11, 2018

bastienpaulfr May 14, 2018

vanniktech May 16, 2018

bastienpaulfr May 16, 2018

vanniktech May 16, 2018

bastienpaulfr May 16, 2018

vanniktech May 16, 2018

bastienpaulfr May 16, 2018

vanniktech May 16, 2018 •

edited

Loading

bastienpaulfr May 16, 2018

vanniktech left a comment

vanniktech left a comment


		compileOnly 'com.android.tools.build:gradle:3.1.2'

		testCompile 'com.android.tools.build:gradle:3.1.2'

Remove need of project.afterEvaluate() and add generateJavadoc macro task #57

Remove need of project.afterEvaluate() and add generateJavadoc macro task #57

Conversation

bastienpaulfr commented May 9, 2018 • edited Loading

codecov bot commented May 9, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanniktech May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanniktech left a comment

Choose a reason for hiding this comment

vanniktech left a comment

Choose a reason for hiding this comment

bastienpaulfr commented May 9, 2018 •

edited

Loading

codecov bot commented May 9, 2018 •

edited

Loading

vanniktech May 16, 2018 •

edited

Loading